Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PM-16154: Learn new login guided tour #1266

Conversation

ezimet-livefront
Copy link
Collaborator

@ezimet-livefront ezimet-livefront commented Jan 14, 2025

🎟️ Tracking

PM-16154
PM-16175
PM-16203

📔 Objective

  • Adds shared guided tour view.
  • Adds 3 steps coach-marks for learn new logins.

📸 Screenshots

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-01-13.at.15.07.56.mp4

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 74.33036% with 115 lines in your changes missing coverage. Please review.

Project coverage is 89.34%. Comparing base (619e70e) to head (7295866).
Report is 1 commits behind head on PM-16153/draw-new-login-action-card.

Files with missing lines Patch % Lines
...lication/Views/GuidedTourView/GuidedTourView.swift 68.10% 89 Missing ⚠️
...on/Views/GuidedTourView/GuidedTourScrollView.swift 51.16% 21 Missing ⚠️
...ion/Views/GuidedTourView/GuidedTourViewState.swift 90.32% 3 Missing ⚠️
...rm/Application/Extensions/View+OnSizeChanged.swift 90.90% 2 Missing ⚠️
Additional details and impacted files
@@                           Coverage Diff                           @@
##           PM-16153/draw-new-login-action-card    #1266      +/-   ##
=======================================================================
- Coverage                                89.48%   89.34%   -0.15%     
=======================================================================
  Files                                      733      741       +8     
  Lines                                    46308    46753     +445     
=======================================================================
+ Hits                                     41440    41770     +330     
- Misses                                    4868     4983     +115     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Logo
Checkmarx One – Scan Summary & Detailse4e2f494-6aa5-4803-8457-631f395d3c73

Great job, no security vulnerabilities found in this Pull Request

Comment on lines 51 to 53
/// The arrow is horizontally positioned at the left side of the spotlight.
/// The position is calculated by dividing the width of the spotlight by 3
/// and placing the arrow at the center of the first part.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice docs!

@ezimet-livefront
Copy link
Collaborator Author

@matt-livefront @KatherineInCode Thank you for your thorough feedback! I've made an effort to address all of your comments. If I overlooked anything, please let me know.

@ezimet-livefront ezimet-livefront requested a review from a team as a code owner January 20, 2025 22:23
Copy link
Collaborator

@matt-livefront matt-livefront left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, just a few additional comments.

The content gets cut off at large text sizes, could we limit the text size or make it scrollable?

IMG_1447

BitwardenShared/Core/Platform/Utilities/Constants.swift Outdated Show resolved Hide resolved
Comment on lines 75 to 80
ScrollViewReader { reader in
ScrollView {
// Dummy spacer view for scroll view to locate when scrolling to top
Spacer()
.frame(height: 0)
.id(Constants.top)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I wonder if we could encapsulate much of the guided tour setup within its own view that then wraps the content in the scroll view?

Here's my thought, this relies on moving showGuidedTour into the state, but otherwise this is pretty self contained and would keep many of the properties and logic for scroll view aspect of the guided tour together and allow for reusability. What do you think?

I also wonder if we could add the full screen cover here or if that needs to be at the top-level?

struct GuidedTourScrollView<Content: View>: View {
    @ViewBuilder var content: Content

    let state: GuidedTourViewState

    /// An environment variable for getting the vertical size class of the view.
    @Environment(\.verticalSizeClass) var verticalSizeClass

    var body: some View {
        ScrollViewReader { reader in
            ScrollView {
                // Dummy spacer view for scroll view to locate when scrolling to top
                Spacer()
                    .frame(height: 0)
                    .id(Constants.top)

                content
            }
            .task(id: verticalSizeClass) {
                handleLandscapeScroll(reader)
            }
            .task(id: state.currentIndex) {
                handleLandscapeScroll(reader)
            }
//            .task(id: store.state.showGuidedTour) {
//                if store.state.showGuidedTour == false {
//                    reader.scrollTo(Constants.top)
//                }
//            }
        }
    }

    /// Scrolls to the guided tour step when in landscape mode.
    private func handleLandscapeScroll(_ reader: ScrollViewProxy) {
        reader.scrollTo(GuidedTourStep(rawValue: state.currentIndex))
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also moved the GuidedTour view to this view.

Copy link
Collaborator

@matt-livefront matt-livefront left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! There's a few lint warnings, could you resolve those?

@ezimet-livefront ezimet-livefront merged commit 632b617 into PM-16153/draw-new-login-action-card Jan 22, 2025
8 checks passed
@ezimet-livefront ezimet-livefront deleted the PM-16154/new-login-coachmark branch January 22, 2025 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants